Conversation
index.js
Outdated
| const path = require('path'); | ||
|
|
||
| const include_dir = path.relative('.', __dirname); | ||
| const include_dir = `"path.relative('.', __dirname)"`; |
There was a problem hiding this comment.
| const include_dir = `"path.relative('.', __dirname)"`; | |
| const include_dir = `"${path.relative('.', __dirname)}"`; |
Isn't this the intended change according to nodejs/node-addon-examples#183 (comment)?
There was a problem hiding this comment.
You are correct. I meant to do that (must've been too tired 😅). Thanks for catching that.
Should be correct now
|
CI run to test across platforms: https://ci.nodejs.org/view/x%20-%20Abi%20stable%20module%20API/job/node-test-node-addon-api-new/4129/ |
|
This seems to fail across all platforms? - https://ci.nodejs.org/view/x%20-%20Abi%20stable%20module%20API/job/node-test-node-addon-api-new/nodes=win-vs2017/4127/ |
|
This looks like the wrong way to fix the examples. Because the packages that already adopted What we should do is to replace list-context expansion with scalar-context one. For example, 1_hello_world would be: diff --git i/1_hello_world/node-addon-api/binding.gyp w/1_hello_world/node-addon-api/binding.gyp
index b819abb..ddf73d2 100644
--- i/1_hello_world/node-addon-api/binding.gyp
+++ w/1_hello_world/node-addon-api/binding.gyp
@@ -6,7 +6,7 @@
"cflags_cc!": [ "-fno-exceptions" ],
"sources": [ "hello.cc" ],
"include_dirs": [
- "<!@(node -p \"require('node-addon-api').include\")"
+ "<!(node -p \"require('node-addon-api').include_dir\")"
],
'defines': [ 'NAPI_DISABLE_CPP_EXCEPTIONS' ],
}
diff --git i/1_hello_world/node-addon-api/package.json w/1_hello_world/node-addon-api/package.json
index f70a69d..fb231dd 100644
--- i/1_hello_world/node-addon-api/package.json
+++ w/1_hello_world/node-addon-api/package.json
@@ -6,7 +6,7 @@
"private": true,
"dependencies": {
"bindings": "~1.2.1",
- "node-addon-api": "^1.0.0"
+ "node-addon-api": "^3.0.2"
},
"scripts": {
"test": "node hello.js" |
|
I see so if we get rid of the |
|
@SurienDG I think it should work on Windows as this repository itself is tested in that way on Windows CI environment. (sorry that I don't have a handy local Windows environment to check it). Even if it doesn't, I suppose we need to fix the gyp's side since it has the responsibility to escape the given paths for each OS and toolchain it supports, and also because changing the node-addon-api side solely for gyp may break packages using the other build tools such as cmake-js. |
|
Ah okay I'll give it a try and then update the examples (if you haven't already). Thanks for your advice @hanazuki |
|
Based on the discussion I think this can be closed. Please let me know if you think that was not the right thing to do. |
include_dirpath inindex.jsso it works correctly on both Windows and linux platforms (as mentioned in C1083: Cannot open include file: 'napi.h': No such file or directory node-addon-examples#183)